Skip to content

Conversation

@eronnen
Copy link
Contributor

@eronnen eronnen commented May 14, 2025

  • Support assembly source breakpoints
  • Change sourceReference to be the symbol load address for simplicity and consistency across threads/frames
Screencast.From.2025-05-17.23-57-30.webm

@eronnen eronnen changed the title Lldb dap assembly breakpoint [lldb-dap] assembly breakpoints May 14, 2025
@eronnen eronnen force-pushed the lldb-dap-assembly-breakpoint branch 2 times, most recently from 2263631 to 3807a12 Compare May 17, 2025 21:43
@eronnen eronnen force-pushed the lldb-dap-assembly-breakpoint branch from 0f2ed75 to f5fd76d Compare May 18, 2025 00:27
@eronnen eronnen marked this pull request as ready for review May 18, 2025 00:27
@eronnen eronnen requested a review from JDevlieghere as a code owner May 18, 2025 00:27
@eronnen eronnen requested a review from ashgti May 18, 2025 00:28
@llvmbot
Copy link
Member

llvmbot commented May 18, 2025

@llvm/pr-subscribers-lldb

Author: Ely Ronnen (eronnen)

Changes
  • Support assembly source breakpoints
  • Change sourceReference to be the load address for simplicity and consistency across threads/frames
Screencast.From.2025-05-17.23-57-30.webm

Patch is 29.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139969.diff

19 Files Affected:

  • (modified) lldb/include/lldb/API/SBFileSpec.h (+3)
  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+7)
  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+10)
  • (modified) lldb/source/API/SBFileSpec.cpp (+8)
  • (added) lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile (+3)
  • (added) lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py (+42)
  • (added) lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c (+14)
  • (modified) lldb/tools/lldb-dap/Breakpoint.cpp (+26-7)
  • (modified) lldb/tools/lldb-dap/DAP.h (+5)
  • (modified) lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp (+51-17)
  • (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+20)
  • (modified) lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp (+75-4)
  • (modified) lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp (+9-11)
  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+54-21)
  • (modified) lldb/tools/lldb-dap/JSONUtils.h (+14)
  • (modified) lldb/tools/lldb-dap/SourceBreakpoint.cpp (+22)
  • (modified) lldb/tools/lldb-dap/SourceBreakpoint.h (+1)
  • (modified) lldb/tools/lldb-dap/package-lock.json (+2-2)
  • (modified) lldb/tools/lldb-dap/package.json (+4-1)
diff --git a/lldb/include/lldb/API/SBFileSpec.h b/lldb/include/lldb/API/SBFileSpec.h
index 36641843aabeb..303cb7d712cbf 100644
--- a/lldb/include/lldb/API/SBFileSpec.h
+++ b/lldb/include/lldb/API/SBFileSpec.h
@@ -10,6 +10,7 @@
 #define LLDB_API_SBFILESPEC_H
 
 #include "lldb/API/SBDefines.h"
+#include "lldb/API/SBStream.h"
 
 namespace lldb {
 
@@ -53,6 +54,8 @@ class LLDB_API SBFileSpec {
 
   uint32_t GetPath(char *dst_path, size_t dst_len) const;
 
+  bool GetPath(lldb::SBStream &dst_path) const;
+
   static int ResolvePath(const char *src_path, char *dst_path, size_t dst_len);
 
   bool GetDescription(lldb::SBStream &description) const;
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 70fd0b0c419db..4a907a5e36901 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -955,6 +955,13 @@ def request_setBreakpoints(self, file_path, line_array, data=None):
         """
         (dir, base) = os.path.split(file_path)
         source_dict = {"name": base, "path": file_path}
+        return self.request_setBreakpoints_with_source(source_dict, line_array, data)
+
+    def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None):
+        source_dict = {"sourceReference": sourceReference}
+        return self.request_setBreakpoints_with_source(source_dict, line_array, data)
+
+    def request_setBreakpoints_with_source(self, source_dict, line_array, data=None):
         args_dict = {
             "source": source_dict,
             "sourceModified": False,
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index afdc746ed0d0d..427f66a7da0c8 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -63,6 +63,16 @@ def set_source_breakpoints(self, source_path, lines, data=None):
         for breakpoint in breakpoints:
             breakpoint_ids.append("%i" % (breakpoint["id"]))
         return breakpoint_ids
+    
+    def set_source_breakpoints_assembly(self, source_reference, lines, data=None):
+        response = self.dap_server.request_setBreakpointsAssembly(source_reference, lines, data)
+        if response is None:
+            return []
+        breakpoints = response["body"]["breakpoints"]
+        breakpoint_ids = []
+        for breakpoint in breakpoints:
+            breakpoint_ids.append("%i" % (breakpoint["id"]))
+        return breakpoint_ids
 
     def set_function_breakpoints(self, functions, condition=None, hitCondition=None):
         """Sets breakpoints by function name given an array of function names
diff --git a/lldb/source/API/SBFileSpec.cpp b/lldb/source/API/SBFileSpec.cpp
index a7df9afc4b8eb..cb44dac1d4fcc 100644
--- a/lldb/source/API/SBFileSpec.cpp
+++ b/lldb/source/API/SBFileSpec.cpp
@@ -19,6 +19,7 @@
 
 #include <cinttypes>
 #include <climits>
+#include <string>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -147,6 +148,13 @@ uint32_t SBFileSpec::GetPath(char *dst_path, size_t dst_len) const {
   return result;
 }
 
+bool SBFileSpec::GetPath(SBStream &dst_path) const {
+  LLDB_INSTRUMENT_VA(this, dst_path);
+
+  std::string path = m_opaque_up->GetPath();
+  return dst_path->PutCString(path.c_str()) > 0;
+}
+
 const lldb_private::FileSpec *SBFileSpec::operator->() const {
   return m_opaque_up.get();
 }
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
new file mode 100644
index 0000000000000..ba9df3a18590b
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
@@ -0,0 +1,42 @@
+"""
+Test lldb-dap setBreakpoints request
+"""
+
+
+import dap_server
+import shutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import os
+
+
+class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase):
+    # @skipIfWindows
+    def test_functionality(self):
+        """Tests hitting assembly source breakpoints"""
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        self.dap_server.request_evaluate(
+            "`settings set stop-disassembly-display no-debuginfo", context="repl"
+        )
+
+        assmebly_func_breakpoints = self.set_function_breakpoints(["assembly_func"])
+        self.continue_to_breakpoints(assmebly_func_breakpoints)
+
+        assembly_func_frame = self.get_stackFrames()[0]
+        self.assertIn(
+            "sourceReference",
+            assembly_func_frame.get("source"),
+            "Expected assembly source frame",
+        )
+
+        line = assembly_func_frame["line"]
+
+        # Set an assembly breakpoint in the next line and check that it's hit
+        assembly_breakpoint_ids = self.set_source_breakpoints_assembly(
+            assembly_func_frame["source"]["sourceReference"], [line + 1]
+        )
+        self.continue_to_breakpoints(assembly_breakpoint_ids)
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c
new file mode 100644
index 0000000000000..350739006f903
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c
@@ -0,0 +1,14 @@
+#include <stddef.h>
+
+__attribute__((nodebug)) int assembly_func(int n) {
+  n += 1;
+  n += 2;
+  n += 3;
+
+  return n;
+}
+
+int main(int argc, char const *argv[]) {
+  assembly_func(10);
+  return 0;
+}
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 26d633d1d172e..a54a34e0f936d 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -9,10 +9,12 @@
 #include "Breakpoint.h"
 #include "DAP.h"
 #include "JSONUtils.h"
+#include "LLDBUtils.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBBreakpointLocation.h"
 #include "lldb/API/SBLineEntry.h"
 #include "lldb/API/SBMutex.h"
+#include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/StringExtras.h"
 #include <cstddef>
 #include <cstdint>
@@ -63,14 +65,31 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
     std::string formatted_addr =
         "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget()));
     breakpoint.instructionReference = formatted_addr;
+
+    lldb::StopDisassemblyType stop_disassembly_display =
+        GetStopDisassemblyDisplay(m_dap.debugger);
     auto line_entry = bp_addr.GetLineEntry();
-    const auto line = line_entry.GetLine();
-    if (line != UINT32_MAX)
-      breakpoint.line = line;
-    const auto column = line_entry.GetColumn();
-    if (column != 0)
-      breakpoint.column = column;
-    breakpoint.source = CreateSource(line_entry);
+    if (!ShouldDisplayAssemblySource(line_entry, stop_disassembly_display)) {
+      const auto line = line_entry.GetLine();
+      if (line != UINT32_MAX)
+        breakpoint.line = line;
+      const auto column = line_entry.GetColumn();
+      if (column != 0)
+        breakpoint.column = column;
+      breakpoint.source = CreateSource(line_entry);
+    } else {
+      // Breakpoint made by assembly
+      auto symbol = bp_addr.GetSymbol();
+      if (symbol.IsValid()) {
+        breakpoint.line =
+            m_bp.GetTarget()
+                .ReadInstructions(symbol.GetStartAddress(), bp_addr, nullptr)
+                .GetSize() +
+            1;
+
+        breakpoint.source = CreateAssemblySource(m_dap.target, bp_addr);
+      }
+    }
   }
 
   return breakpoint;
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 8f24c6cf82924..b0fe265b7bca1 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -169,6 +169,8 @@ struct DAP {
   Variables variables;
   lldb::SBBroadcaster broadcaster;
   llvm::StringMap<SourceBreakpointMap> source_breakpoints;
+  llvm::DenseMap<int64_t, llvm::DenseMap<uint32_t, SourceBreakpoint>>
+      assembly_breakpoints;
   FunctionBreakpointMap function_breakpoints;
   InstructionBreakpointMap instruction_breakpoints;
   std::optional<std::vector<ExceptionBreakpoint>> exception_breakpoints;
@@ -219,6 +221,9 @@ struct DAP {
   llvm::StringSet<> modules;
   /// @}
 
+  /// Number of lines of assembly code to show when no debug info is available.
+  uint32_t number_of_assembly_lines_for_nodebug = 32;
+
   /// Creates a new DAP sessions.
   ///
   /// \param[in] log
diff --git a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
index 2ac886c3a5d2c..c4d658caeee2d 100644
--- a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
@@ -7,7 +7,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
-#include "JSONUtils.h"
 #include "RequestHandler.h"
 #include <vector>
 
@@ -19,19 +18,50 @@ namespace lldb_dap {
 llvm::Expected<protocol::BreakpointLocationsResponseBody>
 BreakpointLocationsRequestHandler::Run(
     const protocol::BreakpointLocationsArguments &args) const {
-  std::string path = args.source.path.value_or("");
   uint32_t start_line = args.line;
   uint32_t start_column = args.column.value_or(LLDB_INVALID_COLUMN_NUMBER);
   uint32_t end_line = args.endLine.value_or(start_line);
   uint32_t end_column =
       args.endColumn.value_or(std::numeric_limits<uint32_t>::max());
 
+  // Find all relevant lines & columns
+  llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations;
+  if (args.source.sourceReference) {
+    AddAssemblyBreakpointLocations(locations, *args.source.sourceReference,
+                                   start_line, end_line);
+  } else {
+    std::string path = args.source.path.value_or("");
+    AddSourceBreakpointLocations(locations, std::move(path), start_line,
+                                 start_column, end_line, end_column);
+  }
+
+  // The line entries are sorted by addresses, but we must return the list
+  // ordered by line / column position.
+  std::sort(locations.begin(), locations.end());
+  locations.erase(llvm::unique(locations), locations.end());
+
+  std::vector<protocol::BreakpointLocation> breakpoint_locations;
+  for (auto &l : locations) {
+    protocol::BreakpointLocation lc;
+    lc.line = l.first;
+    lc.column = l.second;
+    breakpoint_locations.push_back(std::move(lc));
+  }
+
+  return protocol::BreakpointLocationsResponseBody{
+      /*breakpoints=*/std::move(breakpoint_locations)};
+}
+
+template <unsigned N>
+void BreakpointLocationsRequestHandler::AddSourceBreakpointLocations(
+    llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+    std::string path, uint32_t start_line, uint32_t start_column,
+    uint32_t end_line, uint32_t end_column) const {
+
   lldb::SBFileSpec file_spec(path.c_str(), true);
   lldb::SBSymbolContextList compile_units =
       dap.target.FindCompileUnits(file_spec);
 
-  // Find all relevant lines & columns
-  llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations;
   for (uint32_t c_idx = 0, c_limit = compile_units.GetSize(); c_idx < c_limit;
        ++c_idx) {
     const lldb::SBCompileUnit &compile_unit =
@@ -71,22 +101,26 @@ BreakpointLocationsRequestHandler::Run(
       locations.emplace_back(line, column);
     }
   }
+}
 
-  // The line entries are sorted by addresses, but we must return the list
-  // ordered by line / column position.
-  std::sort(locations.begin(), locations.end());
-  locations.erase(llvm::unique(locations), locations.end());
+template <unsigned N>
+void BreakpointLocationsRequestHandler::AddAssemblyBreakpointLocations(
+    llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+    int64_t sourceReference, uint32_t start_line, uint32_t end_line) const {
+  lldb::SBAddress address(sourceReference, dap.target);
+  if (!address.IsValid())
+    return;
 
-  std::vector<protocol::BreakpointLocation> breakpoint_locations;
-  for (auto &l : locations) {
-    protocol::BreakpointLocation lc;
-    lc.line = l.first;
-    lc.column = l.second;
-    breakpoint_locations.push_back(std::move(lc));
-  }
+  lldb::SBSymbol symbol = address.GetSymbol();
+  if (!symbol.IsValid())
+    return;
 
-  return protocol::BreakpointLocationsResponseBody{
-      /*breakpoints=*/std::move(breakpoint_locations)};
+  // start_line is relative to the symbol's start address
+  lldb::SBInstructionList insts = symbol.GetInstructions(dap.target);
+  for (uint32_t i = start_line - 1; i < insts.GetSize() && i <= (end_line - 1);
+       ++i) {
+    locations.emplace_back(i, 1);
+  }
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index e6bccfe12f402..80898d1ee5ef1 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -16,6 +16,7 @@
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -232,6 +233,16 @@ class BreakpointLocationsRequestHandler
   }
   llvm::Expected<protocol::BreakpointLocationsResponseBody>
   Run(const protocol::BreakpointLocationsArguments &args) const override;
+
+  template <unsigned N>
+  void AddSourceBreakpointLocations(
+      llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+      std::string path, uint32_t start_line, uint32_t start_column,
+      uint32_t end_line, uint32_t end_column) const;
+  template <unsigned N>
+  void AddAssemblyBreakpointLocations(
+      llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+      int64_t sourceReference, uint32_t start_line, uint32_t end_line) const;
 };
 
 class CompletionsRequestHandler : public LegacyRequestHandler {
@@ -378,6 +389,15 @@ class SetBreakpointsRequestHandler
   }
   llvm::Expected<protocol::SetBreakpointsResponseBody>
   Run(const protocol::SetBreakpointsArguments &args) const override;
+
+  std::vector<protocol::Breakpoint> SetSourceBreakpoints(
+      const protocol::Source &source,
+      const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+      const;
+  std::vector<protocol::Breakpoint> SetAssemblyBreakpoints(
+      const protocol::Source &source,
+      const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+      const;
 };
 
 class SetExceptionBreakpointsRequestHandler : public LegacyRequestHandler {
diff --git a/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
index 86e090b66afe9..7b401f06e9a85 100644
--- a/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
@@ -23,15 +23,29 @@ llvm::Expected<protocol::SetBreakpointsResponseBody>
 SetBreakpointsRequestHandler::Run(
     const protocol::SetBreakpointsArguments &args) const {
   const auto &source = args.source;
-  const auto path = source.path.value_or("");
   std::vector<protocol::Breakpoint> response_breakpoints;
+  if (source.sourceReference)
+    response_breakpoints = SetAssemblyBreakpoints(source, args.breakpoints);
+  else if (source.path)
+    response_breakpoints = SetSourceBreakpoints(source, args.breakpoints);
+
+  return protocol::SetBreakpointsResponseBody{std::move(response_breakpoints)};
+}
+
+std::vector<protocol::Breakpoint>
+SetBreakpointsRequestHandler::SetSourceBreakpoints(
+    const protocol::Source &source,
+    const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+    const {
+  std::vector<protocol::Breakpoint> response_breakpoints;
+  std::string path = source.path.value_or("");
 
   // Decode the source breakpoint infos for this "setBreakpoints" request
   SourceBreakpointMap request_bps;
   // "breakpoints" may be unset, in which case we treat it the same as being set
   // to an empty array.
-  if (args.breakpoints) {
-    for (const auto &bp : *args.breakpoints) {
+  if (breakpoints) {
+    for (const auto &bp : *breakpoints) {
       SourceBreakpoint src_bp(dap, bp);
       std::pair<uint32_t, uint32_t> bp_pos(src_bp.GetLine(),
                                            src_bp.GetColumn());
@@ -73,7 +87,64 @@ SetBreakpointsRequestHandler::Run(
     }
   }
 
-  return protocol::SetBreakpointsResponseBody{std::move(response_breakpoints)};
+  return response_breakpoints;
+}
+
+std::vector<protocol::Breakpoint>
+SetBreakpointsRequestHandler::SetAssemblyBreakpoints(
+    const protocol::Source &source,
+    const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+    const {
+  std::vector<protocol::Breakpoint> response_breakpoints;
+  int64_t sourceReference = source.sourceReference.value_or(0);
+
+  lldb::SBAddress address(sourceReference, dap.target);
+  if (!address.IsValid())
+    return response_breakpoints;
+
+  lldb::SBSymbol symbol = address.GetSymbol();
+  if (!symbol.IsValid())
+    return response_breakpoints; // Not yet supporting breakpoints in assembly
+                                 // without a valid symbol
+
+  llvm::DenseMap<uint32_t, SourceBreakpoint> request_bps;
+  if (breakpoints) {
+    for (const auto &bp : *breakpoints) {
+      SourceBreakpoint src_bp(dap, bp);
+      request_bps.try_emplace(src_bp.GetLine(), src_bp);
+      const auto [iv, inserted] =
+          dap.assembly_breakpoints[sourceReference].try_emplace(
+              src_bp.GetLine(), src_bp);
+      // We check if this breakpoint already exists to update it
+      if (inserted)
+        iv->getSecond().SetBreakpoint(symbol);
+      else
+        iv->getSecond().UpdateBreakpoint(src_bp);
+
+      protocol::Breakpoint response_bp = iv->getSecond().ToProtocolBreakpoint();
+      response_bp.source = source;
+      if (!response_bp.line)
+        response_bp.line = src_bp.GetLine();
+      if (bp.column)
+        response_bp.column = *bp.column;
+      response_breakpoints.push_back(response_bp);
+    }
+  }
+
+  // Delete existing breakpoints for this sourceReference that are not in the
+  // request_bps set.
+  auto old_src_bp_pos = dap.assembly_breakpoints.find(sourceReference);
+  if (old_src_bp_pos != dap.assembly_breakpoints.end()) {
+    for (auto &old_bp : old_src_bp_pos->second) {
+      auto request_pos = request_bps.find(old_bp.first);
+      if (request_pos == request_bps.end()) {
+        dap.target.BreakpointDelete(old_bp.second.GetID());
+        old_src_bp_pos->second.erase(old_bp.first);
+      }
+    }
+  }
+
+  return response_breakpoints;
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
index 0ddd87881a164..9249e2aa6fef7 100644
--- a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
@@ -11,6 +11,7 @@
 #include "LLDBUtils.h"
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
+#include "lldb/API/SBAddress.h"
 #include "lldb/API/SBExecutionContext.h"
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBInstructionList.h"
@@ -19,6 +20,7 @@
 #include "lldb/API/SBSymbol.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
+#include "lldb/lldb-types.h"
 #include "llvm/Support/Error.h"
 
 namespace lldb_dap {
@@ -34,26 +36,22 @@ SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
     return llvm::make_error<DAPError>(
         "invalid arguments, expected source.sourceReference to be set");
 
-  lldb::SBProcess process = dap.target.GetProcess();
-  // Upper 32 bits is the thread index ID
-  lldb::SBThread thread =
-      process.GetThreadByIndexID(GetLLDBThreadIndexID(source));
- ...
[truncated]

@eronnen eronnen requested a review from da-viper May 18, 2025 00:28
@github-actions
Copy link

github-actions bot commented May 18, 2025

✅ With the latest revision this PR passed the Python code formatter.


uint32_t GetPath(char *dst_path, size_t dst_len) const;

bool GetPath(lldb::SBStream &dst_path) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about adding this API. Generally, we usually use streams when we have potentially multiline output which isn't the case. Here I'm also worried about setting a precedent that will lead to having more methods overloaded with streams. If this were private API I wouldn't mind, but since we guarantee ABI stability for the SB API, once an API has been added, we have to support it forever.

How about adding a helper to LLDBUtils that takes a SBFileSpec and returns the path as a std::string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, I was trying to avoid the potential problem of the path being longer than MAX_PATH but it's still possible to get the real size using GetFilename and GetDirectory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to LLDBUtils

/// @}

/// Number of lines of assembly code to show when no debug info is available.
uint32_t number_of_assembly_lines_for_nodebug = 32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a constant?

Suggested change
uint32_t number_of_assembly_lines_for_nodebug = 32;
static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Comment on lines 106 to 108
template <unsigned N>
void BreakpointLocationsRequestHandler::AddAssemblyBreakpointLocations(
llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid the template by taking a reference to a SmallVectorImpl<std::pair<uint32_t, uint32_t>>, which SmallVector inherits from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at how this is called, maybe you don't even need to take this by reference, and you can have this return a llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8>. Honestly I don't even know if the SmallVector optimization is worth it here, personally I would've just returned a regular std::vector. If you change the return type, you can also change the name of the method from Add to Get. Now it looks like there's potentially multiple calls adding values to the location list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreem chganged to return std::vector

Comment on lines 106 to 108
if (!symbol.IsValid())
return response_breakpoints; // Not yet supporting breakpoints in assembly
// without a valid symbol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about adding the braces when you have a comment.

Suggested change
if (!symbol.IsValid())
return response_breakpoints; // Not yet supporting breakpoints in assembly
// without a valid symbol
if (!symbol.IsValid()) {
// Not yet supporting breakpoints in assembly without a valid symbol.
return response_breakpoints;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

const auto [iv, inserted] =
dap.assembly_breakpoints[sourceReference].try_emplace(
src_bp.GetLine(), src_bp);
// We check if this breakpoint already exists to update it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We check if this breakpoint already exists to update it
// We check if this breakpoint already exists to update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

uint32_t end_column =
args.endColumn.value_or(std::numeric_limits<uint32_t>::max());

// Find all relevant lines & columns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Find all relevant lines & columns
// Find all relevant lines & columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Comment on lines 44 to 49
for (auto &l : locations) {
protocol::BreakpointLocation lc;
lc.line = l.first;
lc.column = l.second;
breakpoint_locations.push_back(std::move(lc));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to create the locations in place like this:

Suggested change
for (auto &l : locations) {
protocol::BreakpointLocation lc;
lc.line = l.first;
lc.column = l.second;
breakpoint_locations.push_back(std::move(lc));
}
for (auto &l : locations)
breakpoint_locations.emplace_back({l.first, l.second});

Copy link
Contributor Author

@eronnen eronnen May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason got no matching constructor for initialization, but the following works

breakpoint_locations.push_back({l.first, l.second, std::nullopt, std::nullopt});

return response_breakpoints; // Not yet supporting breakpoints in assembly
// without a valid symbol

llvm::DenseMap<uint32_t, SourceBreakpoint> request_bps;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment saying what the key is, or better change the variable name to make it obvious: line_to_requested_source_bps or something.

Copy link
Contributor Author

@eronnen eronnen May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to the DAP abstraction

Comment on lines 115 to 116
const auto [iv, inserted] =
dap.assembly_breakpoints[sourceReference].try_emplace(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This together with the logic to either add or update a breakpoint seems like it could be abstracted away behind a function in the dap class. Something like dap.SetAssemblyBreakpoint(uint32_t line, SourceBreakpoint& source_bp). Same thing with the delete below and then we can make assembly_breakpoints a member and provide some more abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Great idea, moved this implementation to a DAP function using the same function for both source and assembly breakpoints

@eronnen eronnen requested a review from JDevlieghere May 18, 2025 11:53
namespace lldb_dap {

typedef llvm::DenseMap<std::pair<uint32_t, uint32_t>, SourceBreakpoint>
typedef std::map<std::pair<uint32_t, uint32_t>, SourceBreakpoint>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to std::map because I'm not sure how to remove from the map while iterating it with DenseMap

Comment on lines 958 to 964
return self.request_setBreakpoints_with_source(source_dict, line_array, data)

def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None):
source_dict = {"sourceReference": sourceReference}
return self.request_setBreakpoints_with_source(source_dict, line_array, data)

def request_setBreakpoints_with_source(self, source_dict, line_array, data=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the request_* names just the DAP requests and make a helper without the request_* prefix for this? I like the consistency of having request_* being able to map to specific request and I think its valuable to keep that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@@ -0,0 +1,55 @@
"""
Test lldb-dap setBreakpoints request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test lldb-dap setBreakpoints in assembly source references?


class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase):
# @skipIfWindows
def test_functionality(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_can_break_in_source_references?

if (source.sourceReference) {
// breakpoint set by assembly source.
lldb::SBAddress source_address(*source.sourceReference, m_dap.target);
if (!source_address.IsValid())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return an error? Or maybe log that this is invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, seems that the DAP client should even display an error if we return it as an invalid breakpoint with a message

Comment on lines +479 to +480
llvm::StringMap<SourceBreakpointMap> m_source_breakpoints;
llvm::DenseMap<int64_t, SourceBreakpointMap> m_source_assembly_breakpoints;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we made protocol::Source work in a std::map (or llvm::DenseMap) as a key, could we merge these? That could simplify things and make this more consistent between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be pretty simplified already since the only access to these maps is abstracted by DAP::SetSourceBreakpoints

It should be possible to use Source as a key but I think it might introduce new complications like implementing a hash / ordering for the struct, and there are extra fields in Source like name and presentationHint

…oints in python tests to support both paths and source_reference
@eronnen eronnen requested review from ashgti and da-viper May 19, 2025 21:06
return self.send_recv(command_dict)

def request_setBreakpoints(self, file_path, line_array, data=None):
def request_setBreakpoints(self, source_dict, line_array, data=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

from typing import TypedDict, Optional

class Source(TypedDict, total=False): # At runtime, this is just a dict with well known keys
  name: str
  path: str
  sourceReference: int


class SourceBreakpoint(TypedDict, total=False):
  line: int
  column: str
  condition: str
  hitCondition: str
  logMessage: str


def request_setBreakpoints(
   self,
   # For backwards compatibility, leaving these as positional args
   sourcePath: Optional[str] = None,
   lines: Optional[list[int]] = None,
   breakpoints: Optional[list[SourceBreakpoint]] = None,
   *,
   source: Optional[Source] = None,
   sourceReference: Optional[int] = None,
):
  source_dict = {}
  if source is not None:
     source_dict = source
  elif sourcePath is not None:
    source_dict["name"] = os.path.basename(sourcePath)
    source_dict["pah"] = sourcePath
  elif sourceReference is not None:
    source_dict["sourceReference"] = sourceReference
  else:
    raise ValueError("'source', 'sourcePath' or 'sourceReference' must be set')

  args_dict = {
    "source": source_dict,
    "sourceModified": False,
  }
  ...

Then we wouldn't need the get_source_for_path and get_source_for_source_reference helpers, since those would be built based on the args of the request.

I could also look at adding this as a follow up to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, I added a simple Source class for this, let me know what you think

@eronnen eronnen requested a review from da-viper May 20, 2025 00:49
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly style nits at this point. The comments and braces are relatively easy to spot, so in the future please do a review pass over your own PRs. I bet you'll be able to catch most of these yourself.

breakpoint.column = column;
breakpoint.source = CreateSource(line_entry);
} else {
// Breakpoint made by assembly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Breakpoint made by assembly
// Assembly breakpoint.

const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints) {
std::vector<protocol::Breakpoint> response_breakpoints;
if (source.sourceReference) {
// breakpoint set by assembly source.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// breakpoint set by assembly source.
// Breakpoint set by assembly source.

response_breakpoints =
SetSourceBreakpoints(source, breakpoints, existing_breakpoints);
} else {
// breakpoint set by a regular source file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// breakpoint set by a regular source file.
// Breakpoint set by a regular source file.

Comment on lines 1670 to 1671
} else
iv->second.UpdateBreakpoint(src_bp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else
iv->second.UpdateBreakpoint(src_bp);
} else {
iv->second.UpdateBreakpoint(src_bp);
}

existing_breakpoints.try_emplace(bp_pos, src_bp);
// We check if this breakpoint already exists to update it.
if (inserted) {
if (auto error = iv->second.SetBreakpoint(source)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use auto: the type isn't obvious from the RHS.

Suggested change
if (auto error = iv->second.SetBreakpoint(source)) {
if (llvm::Error error = iv->second.SetBreakpoint(source)) {

/// @}

/// Number of lines of assembly code to show when no debug info is available.
static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants use a k_ prefix.

Suggested change
static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32;
static constexpr uint32_t k_number_of_assembly_lines_for_nodebug = 32;

Comment on lines 30 to 33
if (args.source.sourceReference)
locations = GetAssemblyBreakpointLocations(*args.source.sourceReference,
start_line, end_line);
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (args.source.sourceReference)
locations = GetAssemblyBreakpointLocations(*args.source.sourceReference,
start_line, end_line);
else {
if (args.source.sourceReference) {
locations = GetAssemblyBreakpointLocations(*args.source.sourceReference,
start_line, end_line);
} else {

if (!symbol.IsValid())
return locations;

// start_line is relative to the symbol's start address
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// start_line is relative to the symbol's start address
// start_line is relative to the symbol's start address.


lldb::SBSymbol symbol = source_address.GetSymbol();
if (!symbol.IsValid()) {
// Not yet supporting breakpoints in assembly without a valid symbol.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Not yet supporting breakpoints in assembly without a valid symbol.
// FIXME: Support assembly breakpoints without a valid symbol.


m_bp = m_dap.target.BreakpointCreateBySBAddress(address);
} else {
// breakpoint set by a regular source file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// breakpoint set by a regular source file.
// Breakpoint set by a regular source file.

@eronnen
Copy link
Contributor Author

eronnen commented May 20, 2025

@JDevlieghere Fixed comments, sorry for the trouble. I'll try to pay extra attention next time :)

@eronnen eronnen requested a review from JDevlieghere May 21, 2025 04:59
@eronnen eronnen merged commit e972d8c into llvm:main May 21, 2025
10 checks passed
@DavidSpickett
Copy link
Collaborator

This is failing on Windows on Arm, started here: https://lab.llvm.org/buildbot/#/builders/141/builds/8891

AssertionError: False is not true : breakpoint not hit, stopped_events=[{'body': {'exitCode': 0}, 'event': 'exited', 'seq': 0, 'type': 'event'}]

Config=aarch64-C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe

Have any fixes been attempted already? I'm going to reproduce locally and see if I can fix it.

@eronnen
Copy link
Contributor Author

eronnen commented May 22, 2025

I Haven't attempted, unfortunately I only own a x64 laptop, I can attempt to reproduce on a VM later

@DavidSpickett
Copy link
Collaborator

Let's see what I find first, sometimes these things are quite simple to fix.

DavidSpickett added a commit that referenced this pull request May 22, 2025
New test added by #139969.

On Windows we need debug information to be able to break on the
function (we don't need it for main, but I assume that's a special case).
So disable the test on Windows.

I tried a few tricks like making a global label in assembly, but
that doesn't show up without debug info either.
@DavidSpickett
Copy link
Collaborator

Looks like a Windows/PDB/COFF vs. Linux/DWARF/ELF difference. We can't break on a function when there's no debug information. It's not due to the architecture.

So if you feel like coming up with a hack to make it work, the failure should reproduce on x64 Windows as well. Personally I'm fine with this just being tested on Linux. The principles are generic enough.

@DavidSpickett
Copy link
Collaborator

Such a hack might be breaking on a place that does have debug information and stepping into one that doesn't, then placing a breakpoint on subsequent assembly lines. Might be. Didn't try it because it seemed like it might defeat the point of the test as it wouldn't place the initial breakpoint on a place without debug info.

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 22, 2025
New test added by llvm/llvm-project#139969.

On Windows we need debug information to be able to break on the
function (we don't need it for main, but I assume that's a special case).
So disable the test on Windows.

I tried a few tricks like making a global label in assembly, but
that doesn't show up without debug info either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants